Conversation
…nfiguration to compile skeleton.
hiker
requested review from
MatthewHambley,
mike-hobson and
stevemullerworth
as code owners
github-actions
bot
added
the
cla-required
github-actions
bot
added
cla-signed
|
This PR only adds a command-line build system using Fab for the Skeleton apps. It is not at all integrated into cylc or any other test suite. Tests have been added to the infrastructure/build/fab scripts which cover the newly added infrastructure files there. Some unit tests are based on some AI input to setup the frame work, but have been manually tweaked to ensure code coverage. Documentation is in form of a README in the above directory, please let me know if you want me to add more elsewhere. |
yaswant
requested review from
t00sa
and removed request for
mike-hobson and
stevemullerworth
MatthewHambley
left a comment
MatthewHambley
left a comment
There was a problem hiding this comment.
I note that a lot of requests made in the original review on SRS have not been addressed so for convenience I have repeated them here.
There was a problem hiding this comment.
Generic build components should be in lfric_build at the top level, not infrastructure/build.
| def process(self, input_template: Path, | ||
| output_file: Path, | ||
| key_values: Dict[str, str]) -> None: | ||
| """ |
There was a problem hiding this comment.
This gives the correct input arguments. Presumably it would be a bad thing to call the run() method on this class. In which case, overriding it with one which throws an exception should provide a bit of safety. You're all ready calling it via super() below so that should be unaffected.
There was a problem hiding this comment.
Generic build components should be in lfric_build at the top level, not infrastructure/build.
| def execute(self, parameters: List[Union[Path, str]]) -> None: | ||
| ''' | ||
| This wrapper adds the required PYTHONPATH, and passes all | ||
| parameters through to the tool's run function. | ||
|
|
||
| :param additional_parameter: A list of parameters for rose picker. | ||
| ''' | ||
| env = os.environ.copy() | ||
| env["PYTHONPATH"] = (f"{env.get('PYTHONPATH', '')}:" | ||
| f"{self._pythonpath}") | ||
|
|
||
| self.run(additional_parameters=parameters, env=env) |
There was a problem hiding this comment.
Providing an alternative entry point with the correct arguments is good. Please override run() with a version which throws an exception to prevent accidental calling.
This will require calls to it within this class to make use of super().
|
|
||
|
|
||
| # ============================================================================= | ||
| def get_rose_picker(tag: str = "v2.0.0") -> RosePicker: |
There was a problem hiding this comment.
What is this for. We expect external tools like Rose Picker to be on the path. If it is, then it's there and can be called. If it isn't, that's an error and the build should stop. I don't understand the need for all this.
|
|
||
| return True | ||
|
|
||
| def execute(self, parameters: List[Union[Path, str]]) -> None: |
There was a problem hiding this comment.
This function should probably return the Path to the files which are generated. Thereby removing the requirement for calling functions to guess.
|
|
||
| :returns List[str]: list of all supported compiler profiles. | ||
| ''' | ||
| return ["full-debug", "fast-debug", "production", "unit-tests"] |
There was a problem hiding this comment.
There's a general problem with there being nothing magical or required about "full debug" or any of the other "profiles." However, specifically there are also integration tests which don't seem to have been covered.
| def setup_cray(self, build_config: BuildConfig) -> None: | ||
| ''' | ||
| This method sets up the Cray compiler and linker flags. | ||
| For now call an external function, since it is expected that | ||
| this configuration can be very lengthy (once we support | ||
| compiler modes). | ||
|
|
||
| :param build_config: the Fab build configuration instance | ||
| :type build_config: :py:class:`fab.BuildConfig` | ||
| ''' | ||
| setup_cray(build_config, self.args) |
There was a problem hiding this comment.
Having a function called setup_cray which calls a function called setup_cray can only ever cause confusion. That goes for all the others. Why does this facade exist?
There was a problem hiding this comment.
Part of the user experience we want to have is that building is always achieved through the same procedure. Specifically, changing into a project directory and issuing a command. The same command. So please rename all build scripts to build. Note that we do not add the .py extension to executables.
|
|
||
| logger = logging.getLogger('fab') | ||
| logger.setLevel(logging.DEBUG) | ||
| fab_skeleton = FabSkeleton(name="skeleton") |
There was a problem hiding this comment.
I notice that the build executable implements a class named after the project being built yet that class is passed the project name on initialisation. Why not pass that name from the constructor?
class ProjectBuild(LFRicBase):
def __init__(self):
super().__init__(name="project")
if __name__ == '__main__':
builder = ProjectBuild()2 participants
PR Summary
Sci/Tech Reviewer: @MatthewHambley
Code Reviewer: @t00sa
Adds a first Fab build script for Skeleton. To keep this change minimal, it's command line only (i.e. no cylc integration, which can come later).
Code Quality Checklist
Testing
trac.log
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review